Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If DebMaker.depends is set, then set it in the control file #288

Merged
merged 2 commits into from
May 29, 2019

Conversation

jacobwil
Copy link
Contributor

This PR makes DebMaker include the depends field in the control file if and only if the field was set.

When the change to no longer include the default depends was made in #209 to fix #208 it entirely eliminated the ability to set the depends programatically. In fact, the field DebMaker.depends is never read.

It does not appear that removing support for setting depends programmatically was intended by the conversations in #209 and #208.

…s set.

When the change to no longer include the default depends was made in tcurdt#209 to fix tcurdt#208 it entirely eliminated the ability to set the depends programatically. In fact, the field DebMaker.depends is never read.
@codecov-io
Copy link

codecov-io commented May 28, 2019

Codecov Report

Merging #288 into master will increase coverage by 0.34%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #288      +/-   ##
============================================
+ Coverage     66.83%   67.18%   +0.34%     
- Complexity       88       91       +3     
============================================
  Files             7        7              
  Lines           579      582       +3     
  Branches         76       78       +2     
============================================
+ Hits            387      391       +4     
+ Misses          142      140       -2     
- Partials         50       51       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/vafer/jdeb/DebMaker.java 59.52% <66.66%> (+0.76%) 45 <0> (+3) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b899a7b...9e50b97. Read the comment docs.

@coveralls
Copy link

coveralls commented May 28, 2019

Coverage Status

Coverage increased (+0.5%) to 75.945% when pulling 9e50b97 on enveil:include-deb-depends-if-set into b899a7b on tcurdt:master.

@tcurdt
Copy link
Owner

tcurdt commented May 29, 2019

Thanks for the PR! Looks good.
But do you think you could also add a little test for this in the DebMakerTest? Thanks!

Similar to https://github.com/tcurdt/jdeb/blob/master/src/test/java/org/vafer/jdeb/DebMakerTestCase.java#L170

Also augment DebMakerTestCase#testDependsIsOmittedWhenEmpty to fail if control is missing entirely.
@jacobwil
Copy link
Contributor Author

Thanks for the PR! Looks good.

Thank you!

But do you think you could also add a little test for this in the DebMakerTest? Thanks!

Sure! Just pushed a new commit with a unit test.

@tcurdt tcurdt merged commit 2a3db71 into tcurdt:master May 29, 2019
@tcurdt
Copy link
Owner

tcurdt commented May 29, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Depends field should be left empty if a package depends on no other
4 participants